This repository was archived by the owner on Apr 12, 2024. It is now read-only.
perf($compile): remove use of deepEquals and $stateful interceptor from bidi bindings #16550
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…om bidi bindings Previously bidirectional bindings were implemented using a custom watch interceptor which stored the component/directive inner value and therefore had to be marked as `$stateful`. The use of a `$stateful` interceptor prevented some optimizations from being used. As a result of using a `$stateful` interceptor the bindings would be executed on each and every digest. For literal expressions this would recreate the literal every digest and therefore require `objectEquality = true`, causing a `angular.equals` call on every digest.
|
I've added a few more tests, including one that failed before the PR ( So far I have failed to find any cases that fail only after the change, but I'm still assuming there are some edge cases that this might "break" so I'd love to get this in before 1.7. |
| parentSet = parentGet.assign || function() { | ||
| // reset the change, or we will throw this exception on every $digest | ||
| lastValue = destination[scopeName] = parentGet(scope); | ||
| throw $compileMinErr('nonassign', | ||
| 'Expression \'{0}\' in attribute \'{1}\' used with directive \'{2}\' is non-assignable!', | ||
| attrs[attrName], attrName, directive.name); | ||
| }; | ||
| var childGet = function childGet() { return destination[scopeName]; }; |
There was a problem hiding this comment.
Isn't the AngularJS coding style to put the var declarations at the top of the function?
There was a problem hiding this comment.
I think normally, but not 2 lines below this 🤷♂️ Preference?
|
Decided not to do this. Too risky at this stage. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
performance, bug fixes (the two
my-component referencetests previously failed)What is the current behavior? (You can also link to an open issue here)
$statefulinterceptor. This means the full expression is executed on every digest due to no input-watching.$statefulinterceptorWhat is the new behavior (if this is a feature change)?
No more
$statefulinterceptors or deep watching when using bidi directive/component inputs.Does this PR introduce a breaking change?
Maybe? But no tests required changing (other then the watcher counts). I thought removing the use of deep equals on literals might change something but I'm not 100% sure...
I guess it might reduce the number of times a literal is evaluated and therefore copy a new instance into the
destinationless frequently. If someone was modifying the internals of the object, those modifications will now be overridden less often? Or maybe previously they would get anoassignerror because the deepEquals detected that change, and now thesimpleComparewon't detect it at all...